fix(kubernetes): single-file GitOps delivery and Kustomize metadata.name exemption - #2874
Open
Erik Osterman (Cloud Posse) (osterman) wants to merge 6 commits into
Open
fix(kubernetes): single-file GitOps delivery and Kustomize metadata.name exemption#2874Erik Osterman (Cloud Posse) (osterman) wants to merge 6 commits into
Erik Osterman (Cloud Posse) (osterman) wants to merge 6 commits into
Conversation
…e objects from metadata.name The `git` provision target always treated its configured `path` as a directory, fanning out one auto-named file per manifest even when the path named an exact file (e.g. `kustomization.yaml`) — creating a directory by that name instead. Kustomize's own `Kustomization`/`Component` objects were also rejected by Atmos's structural validator for lacking `metadata.name`, even though Kustomize's own schema (and its own field-enforcement checks) never requires one. - Add a `split` tri-state on git provision targets: explicit `true`/`false` wins, otherwise inferred from whether `path`'s last segment looks like a manifest filename. `split: false` merges rendered manifests into a single file at the exact path instead of a directory. - Exempt Kustomize's own `Kustomization`/`Component` kinds (matched against their own vendored `sigs.k8s.io/kustomize/api/types` constants) from the `metadata.name` presence check; add an explicit `validate: false` component flag as a general override for the apply/deploy auto-gate and the standalone `validate` command. - Document the new `split` and `validate` fields, and add a full walkthrough for generating a Kustomize component/patch for GitOps delivery.
Required release docs for the split/validate provision-target fix: a problem-first blog post walking through the Kustomize component/GitOps pattern, and a new shipped milestone on the Extensibility roadmap initiative (with a corrected progress percentage).
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
August 5, 2026 17:07
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughThis change adds Kustomize-aware Kubernetes validation controls and optional validation bypass. Git targets support inferred or explicit split mode, including deterministic single-file multi-document YAML output. Shared Git errors now include provider stderr and operation context. ChangesKubernetes validation and configuration propagation
Manifest delivery
Git operation diagnostics
GitOps workflow documentation and delivery feedback
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KubernetesRender
participant GitProvisionTarget
participant writeArtifact
participant MergeYAMLDocuments
participant GitRepository
KubernetesRender->>GitProvisionTarget: rendered artifacts
GitProvisionTarget->>writeArtifact: resolved split mode
writeArtifact->>MergeYAMLDocuments: sorted documents
MergeYAMLDocuments-->>writeArtifact: multi-document YAML
writeArtifact->>GitRepository: write target file or directory
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/component/kubernetes/executor.go`:
- Around line 261-265: Update the validation flow around
resolveComponentValidateEnabled, resolveValidateOptions, and runValidate so
validation options are resolved before the validate:false check and --server
requests still call runValidate against the live cluster. Restrict the skip
result to offline structural validation only, preserving existing behavior
otherwise, and add a regression test covering validate:false with --server.
In `@website/blog/2026-08-05-kustomize-gitops-delivery.mdx`:
- Around line 8-10: Update the Kustomize filename explanation in the blog
content to say that a remote base or component must contain a recognized
reserved kustomization file name, not only kustomization.yaml. Use Kustomize as
the context and mention the supported filenames kustomization.yaml,
kustomization.yml, and Kustomization, while keeping kustomization.yaml as the
example. Preserve the existing point that the name is fixed by Kustomize and not
configurable.
In `@website/docs/stacks/components/kubernetes.mdx`:
- Around line 239-244: Update the Kubernetes delivery-mode documentation near
the `path` and `split` explanation to state that an unset `split` infers
single-file delivery when `path` ends in `.yaml`, `.yml`, or `.json`; otherwise
it defaults to directory delivery. Clarify that users should set `split`
explicitly when they need to override this path-based inference.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07caf576-d657-4dff-a351-701802edd092
📒 Files selected for processing (15)
pkg/component/kubernetes/executor.gopkg/component/kubernetes/executor_test.gopkg/component/kubernetes/render.gopkg/component/kubernetes/validate.gopkg/component/kubernetes/validate_test.gopkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/provisioner/target/git/git.gopkg/provisioner/target/git/git_test.gopkg/provisioner/target/manifest.gopkg/provisioner/target/manifest_test.gowebsite/blog/2026-08-05-kustomize-gitops-delivery.mdxwebsite/docs/cli/commands/kubernetes/kubernetes-deploy.mdxwebsite/docs/cli/commands/kubernetes/kubernetes-validate.mdxwebsite/docs/stacks/components/kubernetes.mdxwebsite/src/data/roadmap.js
Address CodeRabbit review on #2874: - The validate:false short-circuit returned before resolving --server, so `atmos kubernetes validate --server` never reached the live cluster for a component with validate:false. Resolve validate options first and only skip the offline structural check; --server still runs runServerValidate. Adds a regression test. - Blog post overclaimed kustomization.yaml as the only recognized filename; Kustomize also accepts kustomization.yml and Kustomization (confirmed against the vendored dependency). - Applied CodeRabbit's suggested wording clarifying the split-unset path-extension inference in the stack config docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2874 +/- ##
==========================================
+ Coverage 82.75% 82.77% +0.02%
==========================================
Files 1860 1862 +2
Lines 180337 180485 +148
==========================================
+ Hits 149240 149402 +162
+ Misses 23309 23295 -14
Partials 7788 7788
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
Author
|
CodeRabbit (@coderabbitai) full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/component/kubernetes/validate_test.go (1)
107-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for validation-option resolution.
These assertions test five input scenarios. Put them in one table to follow the repository test convention.
As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”
Proposed refactor.
func TestResolveComponentValidateEnabled(t *testing.T) { - assert.True(t, resolveComponentValidateEnabled(nil), "unset defaults to enabled") - assert.True(t, resolveComponentValidateEnabled(map[string]any{}), "unset defaults to enabled") - assert.True(t, resolveComponentValidateEnabled(map[string]any{"validate": true})) - assert.False(t, resolveComponentValidateEnabled(map[string]any{"validate": false})) - assert.True(t, resolveComponentValidateEnabled(map[string]any{"validate": "false"}), "non-bool values are ignored, defaulting to enabled") + tests := []struct { + name string + componentSection map[string]any + want bool + }{ + {"nil defaults to enabled", nil, true}, + {"empty defaults to enabled", map[string]any{}, true}, + {"true enables validation", map[string]any{"validate": true}, true}, + {"false disables validation", map[string]any{"validate": false}, false}, + {"non-boolean defaults to enabled", map[string]any{"validate": "false"}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, resolveComponentValidateEnabled(tt.componentSection)) + }) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/component/kubernetes/validate_test.go` around lines 107 - 113, Refactor TestResolveComponentValidateEnabled into a table-driven test covering the existing five inputs and expected results, including descriptive case names and messages where useful. Iterate over the cases with the repository’s standard subtest pattern while preserving the current validation-option behavior assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/blog/2026-08-05-kustomize-gitops-delivery.mdx`:
- Around line 57-58: Update
website/blog/2026-08-05-kustomize-gitops-delivery.mdx:57-58,
website/docs/stacks/components/kubernetes.mdx:270-271, and
pkg/provisioner/target/git/git_test.go:250-252 to use
kustomize.config.k8s.io/v1alpha1 for Component fixtures. Update
website/docs/cli/commands/kubernetes/kubernetes-validate.mdx:106-113 to document
Kustomization as v1beta1 and Component as v1alpha1 separately, removing the
wildcard API-version description.
---
Nitpick comments:
In `@pkg/component/kubernetes/validate_test.go`:
- Around line 107-113: Refactor TestResolveComponentValidateEnabled into a
table-driven test covering the existing five inputs and expected results,
including descriptive case names and messages where useful. Iterate over the
cases with the repository’s standard subtest pattern while preserving the
current validation-option behavior assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98b624ea-72ff-4b64-addf-5fbc73840f88
📒 Files selected for processing (15)
pkg/component/kubernetes/executor.gopkg/component/kubernetes/executor_test.gopkg/component/kubernetes/render.gopkg/component/kubernetes/validate.gopkg/component/kubernetes/validate_test.gopkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/provisioner/target/git/git.gopkg/provisioner/target/git/git_test.gopkg/provisioner/target/manifest.gopkg/provisioner/target/manifest_test.gowebsite/blog/2026-08-05-kustomize-gitops-delivery.mdxwebsite/docs/cli/commands/kubernetes/kubernetes-deploy.mdxwebsite/docs/cli/commands/kubernetes/kubernetes-validate.mdxwebsite/docs/stacks/components/kubernetes.mdxwebsite/src/data/roadmap.js
… gap Address CodeRabbit full-review findings on #2874: - All Component examples/fixtures used kustomize.config.k8s.io/v1beta1, which is Kustomization's version, not Component's (v1alpha1). Since isKustomizeConfigObject matches exact (apiVersion, kind) pairs, the examples never actually got the metadata.name exemption they claimed. Fixed in the blog post, the kubernetes.mdx walkthrough, and test fixtures; kubernetes-validate.mdx now documents both exact pairs instead of a kustomize.config.k8s.io/* wildcard. - Added TestWriteArtifactSingleFileModeWriteFailure, closing the patch coverage gap Codecov flagged on writeSingleArtifactFile's two new error branches (MkdirAll/WriteFile failure), mirroring the existing split=true failure test.
… schema gap
A field-test pass over the Kustomize GitOps delivery feature found four
real bugs, all fixed here:
- validate: false on a Kubernetes component was silently dropped during
stack processing and never reached any command: internal/exec's
Kubernetes comp-assembly copied provider/paths/manifests/render but
never validate. Threaded it through the full 3-layer merge (global ->
base component -> instance) via the same key-presence-safe
mergeComponentAnySection pattern paths/manifests already use (not
provider's zero-value pattern, which would be unsafe for a bool), plus
the matching --affected diffing and base-component cache entries.
- Git target errors (atmos kubernetes deploy/apply --target <git>) were
always opaque ("git clone (exit 128)", no cause) because the git
provisioner target never captured subprocess stderr, unlike the atmos
git command family. Moved the capture-and-hint machinery from cmd/git
into exported pkg/git symbols (CaptureStderr, WrapOperationError) so
both share one implementation, and wired it into the provisioner's
clone/commit/push calls.
- components.kubernetes.<name>.validate: false failed schema validation
("additionalProperties 'validate' not allowed') because the repo has
three hand-maintained copies of the stack-manifest JSON schema and the
original fix only patched one; atmos describe stacks/validate stacks
enforce a different copy. Patched the copy that's actually enforced and
added a regression test.
- A successful git-target delivery printed nothing at all, unlike cluster
apply and validate. Added a confirmation message.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
pkg/git/errors.go (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd performance tracking to
CaptureStderr.
CaptureStderris an exported operation helper. Adddefer perf.Track(nil, "git.CaptureStderr")()and the required blank line.As per coding guidelines, “Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus a blank line to public functions” unless an explicit exemption applies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/git/errors.go` around lines 29 - 41, Update the exported CaptureStderr function to defer perf.Track(nil, "git.CaptureStderr")() at its start, adding the required blank line after the tracking statement and importing the perf package if necessary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/exec/describe_affected_components.go`:
- Line 593: Update the comparison loop around cfg.ValidateSectionName to detect
changes when the local stack removes validate, even if the remote section still
exists; compare section presence before applying the absent-section skip, while
preserving value comparison for present sections. Add a regression test covering
removal of validate: false and confirming the component is reported as affected.
In `@internal/exec/stack_processor_merge_test.go`:
- Around line 739-779: The merge tests around mergeComponentConfigurations
currently omit the GlobalKubernetesValidate layer. Add focused cases covering
global validate true, global validate false, and precedence across global, base,
and component values, including explicit component and base overrides where
applicable; configure GlobalKubernetesValidate on the test AtmosConfig and
assert the resulting cfg.ValidateSectionName value or absence.
In `@internal/exec/stack_processor_utils.go`:
- Line 2792: Update the Base component validate comment in the surrounding stack
processor utility code to end with a period, preserving its existing wording.
In `@pkg/component/kubernetes/provision_test.go`:
- Around line 105-145: Strengthen TestDeliverApplyPrintsSuccessConfirmation by
asserting that uiOutput also contains the delivered object-count text “delivered
1 Kubernetes object(s)”, while retaining the existing target-name assertion.
In `@pkg/datafetcher/schema_condition_validation_test.go`:
- Around line 196-200: Update the "website" entry in the schemas map within the
relevant test to load the actual website schema file instead of calling
loadWebsiteSchemaBytes, which currently returns the embedded schema. Reuse the
existing website schema file-loading helper or path used elsewhere in the test
package, while leaving the embedded and stack-config entries unchanged.
In `@pkg/git/errors_test.go`:
- Around line 44-70: Refactor the test doubles around stubNonSwappableProvider
so it no longer inherits SwapStderr: introduce a shared base provider without
that method, embed it in both stubSwappableProvider and
stubNonSwappableProvider, and define SwapStderr only on stubSwappableProvider.
Keep TestCaptureStderr_NonSwappableProviderRunsUnmodified exercising the
fallback path.
In `@pkg/provisioner/target/git/git_test.go`:
- Around line 489-498: Remove the exec.LookPath check and all Git CLI setup from
TestDeliverIntegrationCloneErrorSurfacesStderrAndHint. Use the package-level
provider function hook to install a deterministic test double that returns the
clone error and writes representative stderr, then exercise the existing
delivery flow and restore the hook afterward; keep the regression test
unconditional.
---
Nitpick comments:
In `@pkg/git/errors.go`:
- Around line 29-41: Update the exported CaptureStderr function to defer
perf.Track(nil, "git.CaptureStderr")() at its start, adding the required blank
line after the tracking statement and importing the perf package if necessary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce2d2d82-9ab2-4223-a7f9-62362f8c481e
📒 Files selected for processing (22)
cmd/git/executor.gointernal/exec/describe_affected_components.gointernal/exec/stack_processor_cache.gointernal/exec/stack_processor_merge.gointernal/exec/stack_processor_merge_test.gointernal/exec/stack_processor_process_stacks.gointernal/exec/stack_processor_process_stacks_helpers.gointernal/exec/stack_processor_process_stacks_helpers_extraction.gointernal/exec/stack_processor_process_stacks_helpers_inheritance.gointernal/exec/stack_processor_utils.gopkg/component/kubernetes/provision.gopkg/component/kubernetes/provision_test.gopkg/component/kubernetes/validate.gopkg/config/const.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema_condition_validation_test.gopkg/datafetcher/schema_section_coverage_test.gopkg/git/errors.gopkg/git/errors_test.gopkg/provisioner/target/git/git.gopkg/provisioner/target/git/git_test.gopkg/schema/schema.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/component/kubernetes/validate.go
| {sectionNamePaths, affectedReasonStackPaths}, | ||
| {sectionNameManifests, affectedReasonStackManifests}, | ||
| {sectionNameRender, affectedReasonStackRender}, | ||
| {cfg.ValidateSectionName, fmt.Sprintf("stack.%s", cfg.ValidateSectionName)}, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle removal of validate.
The new validate check is added to a loop that skips sections absent from the current component. If the current stack removes validate: false while the remote stack still has it, Lines 605-608 skip the comparison. describe affected then omits the component even though validation changes from disabled to enabled. Compare section presence as well as value and add a regression test for removal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/describe_affected_components.go` at line 593, Update the
comparison loop around cfg.ValidateSectionName to detect changes when the local
stack removes validate, even if the remote section still exists; compare section
presence before applying the absent-section skip, while preserving value
comparison for present sections. Add a regression test covering removal of
validate: false and confirming the component is reported as affected.
Comment on lines
+739
to
+779
| t.Run("validate-component-instance-false-overrides-base-true", func(t *testing.T) { | ||
| opts := ComponentProcessorOptions{ | ||
| ComponentType: cfg.KubernetesComponentType, | ||
| Component: "api", | ||
| AtmosConfig: atmosCfg, | ||
| } | ||
| res := minimalComponentResult() | ||
| res.BaseComponentValidate = true | ||
| res.ComponentValidate = false | ||
| comp, err := mergeComponentConfigurations(atmosCfg, &opts, res) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, false, comp[cfg.ValidateSectionName], | ||
| "an explicit component-instance validate:false must override a base-component validate:true") | ||
| }) | ||
|
|
||
| t.Run("validate-base-true-flows-through-when-component-unset", func(t *testing.T) { | ||
| opts := ComponentProcessorOptions{ | ||
| ComponentType: cfg.KubernetesComponentType, | ||
| Component: "api", | ||
| AtmosConfig: atmosCfg, | ||
| } | ||
| res := minimalComponentResult() | ||
| res.BaseComponentValidate = true | ||
| comp, err := mergeComponentConfigurations(atmosCfg, &opts, res) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, true, comp[cfg.ValidateSectionName], | ||
| "base-component validate:true must flow through when the component instance sets nothing") | ||
| }) | ||
|
|
||
| t.Run("validate-unset-everywhere-is-absent-from-comp", func(t *testing.T) { | ||
| opts := ComponentProcessorOptions{ | ||
| ComponentType: cfg.KubernetesComponentType, | ||
| Component: "api", | ||
| AtmosConfig: atmosCfg, | ||
| } | ||
| res := minimalComponentResult() | ||
| comp, err := mergeComponentConfigurations(atmosCfg, &opts, res) | ||
| require.NoError(t, err) | ||
| _, ok := comp[cfg.ValidateSectionName] | ||
| assert.False(t, ok, "validate must be absent (not defaulted to any value) when unset at every layer") | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the global validation layer.
These cases do not set GlobalKubernetesValidate. Add cases for global true, global false, and precedence against base and component values. This protects the new global-to-component merge contract.
As per coding guidelines, “Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/stack_processor_merge_test.go` around lines 739 - 779, The
merge tests around mergeComponentConfigurations currently omit the
GlobalKubernetesValidate layer. Add focused cases covering global validate true,
global validate false, and precedence across global, base, and component values,
including explicit component and base overrides where applicable; configure
GlobalKubernetesValidate on the test AtmosConfig and assert the resulting
cfg.ValidateSectionName value or absence.
Source: Coding guidelines
| } | ||
| baseComponentConfig.BaseComponentManifests = mergedAny | ||
|
|
||
| // Base component `validate` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
End the new Go comment with a period.
Line 2792 is // Base component \validate`without a final period. Add.` to satisfy repository linting.
As per coding guidelines, “All comments must end with periods.”
Proposed fix.
- // Base component `validate`
+ // Base component `validate`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Base component `validate` | |
| // Base component `validate`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/stack_processor_utils.go` at line 2792, Update the Base
component validate comment in the surrounding stack processor utility code to
end with a period, preserving its existing wording.
Source: Coding guidelines
Comment on lines
+105
to
+145
| // TestDeliverApplyPrintsSuccessConfirmation guards against a successful | ||
| // git/external-target delivery silently producing no output: before this | ||
| // fix, a user running `apply --target <git-target>` against a healthy | ||
| // repository saw nothing at all on success, unlike cluster apply and | ||
| // `validate`. A successful delivery must print a human-facing confirmation. | ||
| func TestDeliverApplyPrintsSuccessConfirmation(t *testing.T) { | ||
| const kind = "test-capture-kind-success-message" | ||
| target.Register(kind, &captureProvisioner{}) | ||
|
|
||
| ioCtx, err := iolib.NewContext() | ||
| require.NoError(t, err) | ||
| ui.InitFormatter(ioCtx) | ||
| t.Cleanup(ui.Reset) | ||
| var uiOutput bytes.Buffer | ||
| restoreUI := iolib.PushUIWriter(&uiOutput) | ||
| t.Cleanup(restoreUI) | ||
|
|
||
| info := &schema.ConfigAndStacksInfo{ | ||
| ComponentFromArg: "argocd", | ||
| Stack: "dev", | ||
| ComponentSection: map[string]any{ | ||
| "provision": map[string]any{ | ||
| "targets": map[string]any{ | ||
| "deployment-repo": map[string]any{ | ||
| "kind": kind, | ||
| "path": "clusters/dev/argocd", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| flags := map[string]any{"target": "deployment-repo"} | ||
| objects := []*unstructured.Unstructured{newObject("Namespace", "atmos-demo")} | ||
|
|
||
| _, err = deliverApply(&schema.AtmosConfiguration{}, info, flags, objects) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Contains(t, uiOutput.String(), "deployment-repo", | ||
| "a successful external-target delivery must print a human-facing confirmation naming the target") | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the delivered object count.
The success message includes the object count and the selected target name. This test checks only the target name. A regression that reports the wrong count will still pass. Add an assertion for delivered 1 Kubernetes object(s) or assert the complete message.
As per coding guidelines, new features must have behavior-focused unit tests that cover the changed behavior.
Suggested assertion
assert.Contains(t, uiOutput.String(), "deployment-repo",
"a successful external-target delivery must print a human-facing confirmation naming the target")
+ assert.Contains(t, uiOutput.String(), "delivered 1 Kubernetes object(s)",
+ "a successful external-target delivery must report the object count")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestDeliverApplyPrintsSuccessConfirmation guards against a successful | |
| // git/external-target delivery silently producing no output: before this | |
| // fix, a user running `apply --target <git-target>` against a healthy | |
| // repository saw nothing at all on success, unlike cluster apply and | |
| // `validate`. A successful delivery must print a human-facing confirmation. | |
| func TestDeliverApplyPrintsSuccessConfirmation(t *testing.T) { | |
| const kind = "test-capture-kind-success-message" | |
| target.Register(kind, &captureProvisioner{}) | |
| ioCtx, err := iolib.NewContext() | |
| require.NoError(t, err) | |
| ui.InitFormatter(ioCtx) | |
| t.Cleanup(ui.Reset) | |
| var uiOutput bytes.Buffer | |
| restoreUI := iolib.PushUIWriter(&uiOutput) | |
| t.Cleanup(restoreUI) | |
| info := &schema.ConfigAndStacksInfo{ | |
| ComponentFromArg: "argocd", | |
| Stack: "dev", | |
| ComponentSection: map[string]any{ | |
| "provision": map[string]any{ | |
| "targets": map[string]any{ | |
| "deployment-repo": map[string]any{ | |
| "kind": kind, | |
| "path": "clusters/dev/argocd", | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| flags := map[string]any{"target": "deployment-repo"} | |
| objects := []*unstructured.Unstructured{newObject("Namespace", "atmos-demo")} | |
| _, err = deliverApply(&schema.AtmosConfiguration{}, info, flags, objects) | |
| require.NoError(t, err) | |
| assert.Contains(t, uiOutput.String(), "deployment-repo", | |
| "a successful external-target delivery must print a human-facing confirmation naming the target") | |
| } | |
| // TestDeliverApplyPrintsSuccessConfirmation guards against a successful | |
| // git/external-target delivery silently producing no output: before this | |
| // fix, a user running `apply --target <git-target>` against a healthy | |
| // repository saw nothing at all on success, unlike cluster apply and | |
| // `validate`. A successful delivery must print a human-facing confirmation. | |
| func TestDeliverApplyPrintsSuccessConfirmation(t *testing.T) { | |
| const kind = "test-capture-kind-success-message" | |
| target.Register(kind, &captureProvisioner{}) | |
| ioCtx, err := iolib.NewContext() | |
| require.NoError(t, err) | |
| ui.InitFormatter(ioCtx) | |
| t.Cleanup(ui.Reset) | |
| var uiOutput bytes.Buffer | |
| restoreUI := iolib.PushUIWriter(&uiOutput) | |
| t.Cleanup(restoreUI) | |
| info := &schema.ConfigAndStacksInfo{ | |
| ComponentFromArg: "argocd", | |
| Stack: "dev", | |
| ComponentSection: map[string]any{ | |
| "provision": map[string]any{ | |
| "targets": map[string]any{ | |
| "deployment-repo": map[string]any{ | |
| "kind": kind, | |
| "path": "clusters/dev/argocd", | |
| }, | |
| }, | |
| }, | |
| }, | |
| } | |
| flags := map[string]any{"target": "deployment-repo"} | |
| objects := []*unstructured.Unstructured{newObject("Namespace", "atmos-demo")} | |
| _, err = deliverApply(&schema.AtmosConfiguration{}, info, flags, objects) | |
| require.NoError(t, err) | |
| assert.Contains(t, uiOutput.String(), "deployment-repo", | |
| "a successful external-target delivery must print a human-facing confirmation naming the target") | |
| assert.Contains(t, uiOutput.String(), "delivered 1 Kubernetes object(s)", | |
| "a successful external-target delivery must report the object count") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/component/kubernetes/provision_test.go` around lines 105 - 145,
Strengthen TestDeliverApplyPrintsSuccessConfirmation by asserting that uiOutput
also contains the delivered object-count text “delivered 1 Kubernetes
object(s)”, while retaining the existing target-name assertion.
Source: Coding guidelines
Comment on lines
+196
to
+200
| schemas := map[string][]byte{ | ||
| "embedded": loadEmbeddedSchemaBytes(t), | ||
| "website": loadWebsiteSchemaBytes(t), | ||
| "stack-config": loadStackConfigSchemaBytes(t), | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Load the website schema for the website case.
loadWebsiteSchemaBytes returns loadEmbeddedSchemaBytes at Lines 333-335. This test validates the embedded schema twice. It does not detect drift in the website schema. Load the website schema file instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/datafetcher/schema_condition_validation_test.go` around lines 196 - 200,
Update the "website" entry in the schemas map within the relevant test to load
the actual website schema file instead of calling loadWebsiteSchemaBytes, which
currently returns the embedded schema. Reuse the existing website schema
file-loading helper or path used elsewhere in the test package, while leaving
the embedded and stack-config entries unchanged.
Comment on lines
+44
to
+70
| // stubNonSwappableProvider implements Provider but not StderrSwapper, mirroring | ||
| // test doubles elsewhere in the codebase that don't need stderr capture. | ||
| type stubNonSwappableProvider struct{ stubSwappableProvider } | ||
|
|
||
| func TestCaptureStderr_SwappableProvider(t *testing.T) { | ||
| provider := &stubSwappableProvider{writeOnSwap: "fatal: something went wrong\n"} | ||
|
|
||
| stderr, err := CaptureStderr(provider, func() error { | ||
| return errors.New("boom") | ||
| }) | ||
|
|
||
| require.Error(t, err) | ||
| assert.Equal(t, "fatal: something went wrong", stderr, "captured stderr must be trimmed") | ||
| } | ||
|
|
||
| func TestCaptureStderr_NonSwappableProviderRunsUnmodified(t *testing.T) { | ||
| var nonSwappable Provider = &struct{ stubNonSwappableProvider }{} | ||
|
|
||
| ran := false | ||
| stderr, err := CaptureStderr(nonSwappable, func() error { | ||
| ran = true | ||
| return nil | ||
| }) | ||
|
|
||
| require.NoError(t, err) | ||
| assert.Empty(t, stderr) | ||
| assert.True(t, ran, "operation must still run when the provider doesn't support stderr capture") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the embedded test types and promoted SwapStderr method.
rg -n -C 3 'stubNonSwappableProvider|stubSwappableProvider|SwapStderr|TestCaptureStderr_NonSwappable' pkg/git/errors_test.goRepository: cloudposse/atmos
Length of output: 2221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== errors.go outline =="
ast-grep outline pkg/git/errors.go --view expanded || true
echo "== pkg/git/errors.go relevant =="
cat -n pkg/git/errors.go | sed -n '1,110p'
echo "== pkg/git/errors_test.go relevant =="
cat -n pkg/git/errors_test.go | sed -n '1,90p'
echo "== Go embedded-method promotion probe =="
cat > /tmp/promotion_probe.go <<'EOF'
package main
import (
"fmt"
"reflect"
)
type SwapStderr interface { SwapStderr() }
type embedded struct{}
func (e *embedded) SwapStderr() {}
type promoted struct{ embedded }
func main() {
var p interface{} = (*promoted)(nil)
t := reflect.TypeOf(p)
fmt.Println("SwapStderr method:", t.MethodByName("SwapStderr"))
fmt.Println("implements SwapStderr:", t.Implements(reflect.TypeOf((*SwapStderr)(nil)).Elem()))
}
EOF
go run /tmp/promotion_probe.goRepository: cloudposse/atmos
Length of output: 142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== provider files =="
fd -t f 'errors\.(go|_test\.go)$' pkg/git || true
echo "== pkg/git/errors.go relevant =="
if [ -f pkg/git/errors.go ]; then
cat -n pkg/git/errors.go | sed -n '1,120p'
fi
echo "== pkg/git/errors_test.go relevant =="
if [ -f pkg/git/errors_test.go ]; then
cat -n pkg/git/errors_test.go | sed -n '1,100p'
fi
echo "== Go embedded-method promotion probe =="
if command -v go >/dev/null 2>&1; then
cat > /tmp/promotion_probe.go <<'EOF'
package main
import (
"fmt"
"io"
"reflect"
"context"
)
type InitOptions struct{}
type CloneOptions struct{}
type PullOptions struct{}
type PushOptions struct{}
type StatusOptions struct{}
type StatusResult struct{}
type DiffOptions struct{}
type DiffResult struct{}
type CommitOptions struct{}
type CommitResult struct{}
type StderrSwapper interface { SwapStderr(io.Writer) func() }
type embedded struct{}
func (e *embedded) SwapStderr(io.Writer) func() { return func() {} }
func (e *embedded) Init(context.Context, *InitOptions) error { return nil }
type promoted struct{ embedded }
func main() {
var p interface{} = (*promoted)(nil)
t := reflect.TypeOf(p)
_, ok := t.MethodByName("SwapStderr")
fmt.Println("SwapStderr method found:", ok)
fmt.Println("implements StderrSwapper:", t.Implements(reflect.TypeOf((*StderrSwapper)(nil)).Elem()))
}
EOF
go run /tmp/promotion_probe.go
else
echo "go not available"
fiRepository: cloudposse/atmos
Length of output: 1964
Make the non-swappable test provider truly non-swappable.
stubNonSwappableProvider embeds stubSwappableProvider, so Go promotes its SwapStderr method to the anonymous struct cast to Provider. This test still exercises the StderrSwapper path instead of the fallback behavior. Define a shared base provider without SwapStderr, embed that base in both test types, and add SwapStderr only to stubSwappableProvider.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/git/errors_test.go` around lines 44 - 70, Refactor the test doubles
around stubNonSwappableProvider so it no longer inherits SwapStderr: introduce a
shared base provider without that method, embed it in both stubSwappableProvider
and stubNonSwappableProvider, and define SwapStderr only on
stubSwappableProvider. Keep TestCaptureStderr_NonSwappableProviderRunsUnmodified
exercising the fallback path.
Comment on lines
+489
to
+498
| func TestDeliverIntegrationCloneErrorSurfacesStderrAndHint(t *testing.T) { | ||
| if _, err := exec.LookPath("git"); err != nil { | ||
| t.Skip("git binary not available") | ||
| } | ||
| isolatedGitEnv(t) | ||
|
|
||
| root := t.TempDir() | ||
| bare := filepath.Join(root, "empty.git") | ||
| gitCmd(t, "", "init", "--bare", bare) | ||
| gitCmd(t, bare, "symbolic-ref", "HEAD", "refs/heads/main") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the external Git dependency from this regression test.
Line 490 skips the test when git is unavailable. Lines 497-498 invoke the Git binary. This can let the suite pass without testing the clone-error contract.
Use a deterministic provider test double through the package test seam. Make the double return the clone error and write representative stderr. Do not skip this regression test.
As per coding guidelines, “Never use platform-specific binaries or shell commands in tests.” Based on learnings, keep the established package-level function-hook testability convention for pkg/** changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/provisioner/target/git/git_test.go` around lines 489 - 498, Remove the
exec.LookPath check and all Git CLI setup from
TestDeliverIntegrationCloneErrorSurfacesStderrAndHint. Use the package-level
provider function hook to install a deterministic test double that returns the
clone error and writes representative stderr, then exercise the existing
delivery flow and restore the hook afterward; keep the regression test
unconditional.
Sources: Coding guidelines, Learnings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
kubernetes.gitops.provision.targets.<name>(kind: git) now supports asplittri-state:split: falsewritespathas a single merged multi-document YAML file instead of always treatingpathas a directory of auto-named files; unset infers the mode from whetherpath's last segment looks like a manifest filename (.yaml/.yml/.json).metadata.nameon Kustomize's ownKustomization/Componentobjects (matched againstsigs.k8s.io/kustomize/api/types's own kind/version constants), since Kustomize's own schema and field-enforcement never require one.validate: falsecomponent-level flag opts a component out of both the apply/deploy structural auto-gate and the standaloneatmos kubernetes validatecommand.splitdocumented onkubernetes-deploy.mdx, and the Kustomize exemption /validate: falsedocumented onkubernetes-validate.mdx.why
pathwas always treated as a directory, so configuringpath: ".../kustomization.yaml"created a directory by that name containing an auto-generated file inside it, instead of the exact file Kustomize's remote-include mechanism requires.metadata.nameunconditionally, forcing users to add a meaningless name to KustomizeComponent/Kustomizationobjects just to satisfy Atmos, even though Kustomize's own tooling never requires one.!terraform.state) and committing it to a deployment repo as a properkustomization.yamlfor Argo CD/Flux to consume.references
Summary by CodeRabbit
New Features
split, with automatic mode selection based on the destination path.KustomizationandComponentobjects no longer requiremetadata.name.validatesetting to bypass offline structural validation.Bug Fixes
Documentation